-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Introduce a dedicated ProjectClient interface
#132237
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
A dedicated `ProjectClient` class will make it easier to guarantee a project scope on a type/compilation basis.
server/src/main/java/org/elasticsearch/client/internal/support/AbstractClient.java
Outdated
Show resolved
Hide resolved
| // We construct a dedicated project client for the default project if we're in single project mode. This dedicated project | ||
| // client is an optimization in that it does not use the project resolver and instead executes the request directly. | ||
| if (defaultProjectClient == null) { | ||
| defaultProjectClient = new ProjectClient(this, projectId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to avoid introducing a performance regression in single-project mode by having to create a new client on every invocation, so I went for a non-final field. Other suggestions are welcome too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can also pro-actively create it at instantiation time similar to AdminClient. In a non-MP setup, it is guaranteed that some code will need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember why I had to construct the default project client lazily: because constructing it in the constructor results in a cyclic dependency/stack overflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it still a problem if we create defaultProjectClient at the end of the constructor? I'd prefer to have the field being final if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that we're constructing the ProjectClient inside the constructor of AbstractClient, but ProjectClient itself also extends AbstractClient. So the place inside the constructor doesn't matter much - if I understand your suggestion correct.
I just realized that we could probably construct the default project client conditionally, only if this is not an instance of ProjectClient (as we'll never need a default project client in that case anyway). What do you think of that solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, calling projectResolver.supportsMultiProjects() in the constructor is making a lot of tests fail because they use TestProjectResolvers.alwaysThrow(). TBH, I'm more inclined to remove the supportsMultiProjects() call instead of updating all those tests. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer updating tests instead of bending production code for them unless there is more unexpected complexity? It's a redundant field in MP and it makes sense to be null. Or I guess someone would probably try to refactor it that way in future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm ok, and I assume you're also no fan of changing supportsMultiProjects() in TestProjectResolvers.alwaysThrow() to return false instead of throwing? I'm just trying to find a way to avoid having to change dozens of test classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep unless we rename alwaysThrow to something else to reflect its new behaviour. To help with progress here, we can drop the projectResolver.supportsMultiProjects check in the constructor in this PR and get back to it in a follow-up? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me 👍. Done in f371275.
| when(client.projectClient(any())).thenReturn(client); | ||
| AdminClient adminClient = mock(AdminClient.class); | ||
| IndicesAdminClient indicesAdminClient = mock(IndicesAdminClient.class); | ||
| when(adminClient.indices()).thenReturn(indicesAdminClient); | ||
| when(client.admin()).thenReturn(adminClient); | ||
| doAnswer(withResponse(AcknowledgedResponse.TRUE)).when(indicesAdminClient).putTemplate(any(), any()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines weren't necessary. The same goes for the watcher test class below.
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a question about class vs interface.
| * A dedicated {@link Client} that is scoped to a specific project. It will set the project ID in the thread context before executing any | ||
| * requests. | ||
| */ | ||
| public class ProjectClient extends FilterClient { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had thought the ProjectClient would be an interface rather than a concrete class, e.g. something like
public interface ProjectClient extends Client {
ProjectId getProjectId();
}
Having a concrete ProjectClient class that extends FilterClient feels a bit odd to me since they are conceptually different things. Having the FilterClient as the implementation and returned from the projectClient() method makes more sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a concrete ProjectClient class that extends FilterClient feels a bit odd to me since they are conceptually different things.
Why are they conceptually different things to you? All that a ProjectClient does is add the project ID in the thread context, which is exactly what a FilterClient is for. ProjectClient doesn't need to extend FilterClient. It can extend AbstractClient as well, but extending FilterClient reduced a little bit of boilerplate code. Can you explain why you think they are conceptually different but you do think using a FilterClient at runtime does make sense?
IMO, a ProjectClient interface like that doesn't really dictate anything about the client. The only method it provides is the getter. Any usages of the interface will just have to "trust" that an implementing class does the project scoping in the requests. In other words, the interface doesn't really provide any guarantee. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All that a ProjectClient does is add the project ID in the thread context
I'd think this is an implementation detail and not what the interface should care. To me, ProjectClient is a business concept which really just means this client is supposed to work with a single project (i.e. it's scope, denoted by its interface face getProjectId()), while FilterClient is an implementation choice. Like you said, it does not even have to be implemented with FilterClient.
the interface doesn't really provide any guarantee
I am not sure whether this really is a problem. We can argue that Client#filterWithHeader method does not provide any guarantee either. The contract can be in the javadoc (JDK's own interfaces do that). If we do feel some more hints are needed, we can augument the interface to be something like the followings:
interface ProjectClient extends Client {
ProjectId getProjectId();
void executeOnProject();
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, you convinced me. Thanks for explaining. I converted ProjectClient to an interface in 63e371b.
server/src/main/java/org/elasticsearch/client/internal/support/AbstractClient.java
Outdated
Show resolved
Hide resolved
| // We construct a dedicated project client for the default project if we're in single project mode. This dedicated project | ||
| // client is an optimization in that it does not use the project resolver and instead executes the request directly. | ||
| if (defaultProjectClient == null) { | ||
| defaultProjectClient = new ProjectClient(this, projectId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can also pro-actively create it at instantiation time similar to AdminClient. In a non-MP setup, it is guaranteed that some code will need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
One comment on whether we can have the defaultProjectClient filed to be final by creating it (default or null) in the constructor.
server/src/main/java/org/elasticsearch/client/internal/support/AbstractClient.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
ProjectClient classProjectClient interface
A dedicated
ProjectClientinterface will make it easier to guarantee a project scope on a type/compilation basis.